-
Notifications
You must be signed in to change notification settings - Fork 2
feat/river schema v2 compatibility #106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
a59a4df to
dadffb8
Compare
masad-frost
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you wanna set up a different base branch in case we need to send a patch against main?
dadffb8 to
4c0e071
Compare
If we need to patch, we should do feature patches off previous release tags. Since we're reserving 1.x for GA, I can follow the 0.200.0 strategy in this repo for schema-v2 releases. Doing that though would suggest that we do at least a halfway decent pass at the v2 server codegen as well though. Given what we discussed supporting v1/v2 using a naive shim doesn't seem too bad, probably can get that done this week. Wdyt? |
b71a809 to
717cc68
Compare
aa49136 to
747f7e1
Compare
Discovered that it was overloaded, written to by multiple different sources with different semantics.
bb52361 to
7123e82
Compare
8589484 to
ffc10ce
Compare
ffc10ce to
4820ef8
Compare
|
Five remaining flakes in babel: |
Why
River Schema v2 compatibility so we can start getting rid of the compatibility layer.
What changed
Client
Significantly restructured the
river.v2.*.asyncio.Lock's have been removedsend_messagewhen called concurrently, so we'd end up withseqbound and incremented but then messages would become out-of-order by the time thews.sendmethod returned. Now we just atomically append to adequeas we push onto the send buffer, and when we get a confirmation from the ws library we move the message over to an ack buffer (also adeque) waiting for incoming messages from the server to allow us to drain.Codegen
initis now required for all method types,inputis now optional. This simplifiesrpcandstreamcodegen.initwas being used ininputposition and vice versa.f"'{foo}'"encoding forf"{repr(foo)}", giving greater safety post-generation (to avoid situations wheref"'{None}'"gets rendered as"None".f"{repr(None)}"would render asNonewhich would fail typechecks.)Test plan
Manually ran codegen against v2 generated schemas, everything typechecks.